Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide semantic conventions for RPC metrics #1162

Merged
merged 5 commits into from
Nov 26, 2020
Merged

Provide semantic conventions for RPC metrics #1162

merged 5 commits into from
Nov 26, 2020

Conversation

jsuereth
Copy link
Contributor

Fixes #1016

Changes

Adds semantic conventions for RPC metrics.

  • Re-use existing RPC attribute spec YAML. Note: this interacts oddly pulling from trace => metrics.
  • Define 3 (x 2) key metrics for RPC: rpc.?.duration, rpc.?.request_size and rpc.?.response_size, crossed by server or client
  • Add warning around reducing cardinality in attributes for metrics (cross-reference the same information for spans).

Deviations from OpenCensus

  • This does not track a "count" of messages that have been issued.
  • This does not pull different metrics per-method vs. per-rpc, aggregation across labels is assumed.

@jsuereth jsuereth requested review from a team October 30, 2020 17:24
@justinfoote justinfoote mentioned this pull request Nov 3, 2020
Closed
justinfoote
justinfoote approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@justinfoote justinfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.
My only question is: should the rpc metrics also include HTTP labels if the RPC calls are happening over HTTP?

@MrAlias MrAlias mentioned this pull request Nov 10, 2020
Closed
@jsuereth
Copy link
Contributor Author

@justinfoote Regarding RPC having HTTP labels if it's over HTTP. While Personally I could see reasons to include it or not, I think perhaps we should follow from TRACE: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/rpc.md#distinction-from-http-spans

TL;DR: The metrics is more about the fact that it's an RPC then it is about HTTP GET/POST/PUT/DELETE resources, so we shouldn't cross-report.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 19, 2020
@jsuereth
Copy link
Contributor Author

@MrAlias Is there anything else I need to do for this PR?

MrAlias
MrAlias approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@github-actions github-actions bot removed the Stale label Nov 20, 2020
@jsuereth
Copy link
Contributor Author

jsuereth commented Nov 24, 2020 •
edited
Loading

@carlosalberto What else do I need to do for this PR?

arminru
arminru reviewed Nov 24, 2020
View reviewed changes
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an entry in CHANGELOG.md for your new semantic convention. Thanks!

specification/metrics/semantic_conventions/rpc.md Show resolved Hide resolved
specification/metrics/semantic_conventions/rpc.md Show resolved Hide resolved
specification/metrics/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
specification/metrics/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
specification/metrics/semantic_conventions/rpc.md Show resolved Hide resolved
@arminru
Copy link
Member

arminru commented Nov 24, 2020

@jsuereth The build checks required to pass for merging are currently saying "Waiting for status to be reported" because we switched to Github actions in the meantime. If you merge or rebase onto the current master, those changes should be picked up and the build should pass.

@jsuereth
Copy link
Contributor Author

Please add an entry in CHANGELOG.md for your new semantic convention. Thanks!

Done!

@arminru arminru merged commit a519397 into open-telemetry:master Nov 26, 2020
@jsuereth jsuereth deleted the wip-rpc-metric-conventions branch April 17, 2021 15:47
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@jsuereth @arminru
Provide semantic conventions for RPC metrics (open-telemetry#1162)
0c3207c 
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arminru arminru arminru left review comments

@jmacd jmacd jmacd approved these changes

@yurishkuro yurishkuro yurishkuro left review comments

@MrAlias MrAlias MrAlias approved these changes

@justinfoote justinfoote justinfoote approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

Assignees

@carlosalberto carlosalberto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define metric semantic conventions for RPC
7 participants
@jsuereth @arminru @carlosalberto @justinfoote @yurishkuro @jmacd @MrAlias